-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/3990 buffer emit remainder on complete #6042
Fix/3990 buffer emit remainder on complete #6042
Conversation
5bdc880
to
e835f92
Compare
@@ -16,11 +17,25 @@ describe('Observable.prototype.buffer', () => { | |||
testScheduler.run(({ hot, expectObservable }) => { | |||
const a = hot(' -a-b-c-d-e-f-g-h-i-|'); | |||
const b = hot(' -----B-----B-----B-|'); | |||
const expected = '-----x-----y-----z-|'; | |||
const expected = '-----x-----y-----z-(F|)'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you're going to see through a lot of these tests is I had to add the last emission of an empty buffer for cases when the closingNotifier
was not complete when the source completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but TBH I am a little disappointed that you didn't use an emoji:
const a = hot(' -a-b-c-d-e-f-g-h-i-|');
const b = hot(' -----B-----B-----B-|');
const expected = '-----x-----y-----z-(🦖|)';
const expectedValues = {
x: ['a', 'b', 'c'],
y: ['d', 'e', 'f'],
z: ['g', 'h', 'i'],
'🦖': [],
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'm going to push another commit to this branch to add some tests that assert/document what I recall you mentioning should be an equivalence between buffer
and window
(with toArray
applied). However, one of the cases in the tests fails. If this equivalence is expected, perhaps we should looks at what's different? I don't have time to do this, right now - I can look at it tomorrow. Feel free to toss the commit if it's misguided or or whatever. I won't be offended. 🙂
@cartant it looks like we caught another inconsistency with our API. The (It's a bug of the same type, basically) |
…bservable Resolves an issue where the resulting observable would complete when the closingNotifier completed. Notifier completion should not complete the result, only source completion should do that. BREAKING CHANGE: closingNotifier no longer closes the result of `buffer`. If that is truly a desired behavior, then you should use `takeUntil`. Something like: `source$.pipe(buffer(notifier$), takeUntil(notifier$.pipe(ignoreElements(), endWith(true))))`, where `notifier$` is multicast, although there are many ways to compose this behavior.
BREAKING CHANGE: Final buffered values will now always be emitted. To get the same behavior as the previous release, you can use `endWith` and `skipLast(1)`, like so: `source$.pipe(buffer(notifier$.pipe(endWith(true))), skipLast(1))` Fixes ReactiveX#3990, ReactiveX#6035
Resolves an issue where the windowBoundary complete would complete the resulting observable. BREAKING CHANGE: The windowBoundaries observable no longer completes the result. It was only ever meant to notify of the window boundary. To get the same behavior as the old behavior, you would need to add an `endWith` and a `skipLast(1)` like so: `source$.pipe(window(notifier$.pipe(endWith(true))), skipLast(1))`.
48677c5
to
0b07c86
Compare
@cartant So I've added another commit that also fixes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just some nitpicks.
Merging despite |
fix(buffer): Remaining buffer will be emited on source close if
closingNotifier
activeGives the author control over the emission of the final buffer. If the
closingNotifier
completes before the source does, no more buffers will be emitted. If theclosingNotifier
is still active when the source completes, then whatever is in the buffer at the time will be emitted from the resulting observable before it completes.BREAKING CHANGE: Final buffered values will now be emitted from the resulting observable if the
closingNotifier
is still active. The simplest workaround if you want the original behavior (where you possibly miss values), is to add askipLast(1)
at the end. Otherwise, you can try to complete theclosingNotifier
prior to the completion of the source.Fixes #3990, #6035
fix(buffer): closingNotifier completion does not complete resulting observable
Resolves an issue where the resulting observable would complete when the closingNotifier completed. Notifier completion should not complete the result, only source completion should do that.
BREAKING CHANGE: closingNotifier no longer closes the result of
buffer
. If that is truly a desired behavior, then you should usetakeUntil
. Something like:source$.pipe(buffer(notifier$), takeUntil(notifier$.pipe(ignoreElements(), endWith(true))))
, wherenotifier$
is multicast, although there are many ways to compose this behavior.